From 4386f99626e02b89fecc30857aab38a6addd1230 Mon Sep 17 00:00:00 2001 From: tsteven4 <13596209+tsteven4@users.noreply.github.com> Date: Wed, 23 Aug 2023 10:32:23 -0600 Subject: [PATCH] resolve maintanence issue in interpolate & resample filters. (#1161) * resolve maintanence issue in interpolate filter. Instead of manually creating a deep copy of the route list with an empty waypoint list we operate on the original route list by swapping it's waypoint list with an empty list. This is both more efficient and easier to maintain. * resolve maintanence issues in resample filter. Instead of manually creating a deep copy of the route list with an empty waypoint list we operate on the original route list by swapping it's waypoint list with an empty list. This is both more efficient and easier to maintain. For efficient deletion use track_del_marked_wpts. --- interpolate.cc | 193 +++++++++++++++++++++++-------------------------- interpolate.h | 8 +- resample.cc | 191 +++++++++++++++++++----------------------------- resample.h | 9 ++- 4 files changed, 182 insertions(+), 219 deletions(-) diff --git a/interpolate.cc b/interpolate.cc index 09fcebc54..0148c52fc 100644 --- a/interpolate.cc +++ b/interpolate.cc @@ -1,7 +1,7 @@ /* Interpolate filter - Copyright (C) 2002 Robert Lipe, robertlipe+source@gpsbabel.org + Copyright (C) 2002,2023 Robert Lipe, robertlipe+source@gpsbabel.org This program is free software; you can redistribute it and/or modify it under the terms of the GNU General Public License as published by @@ -30,7 +30,6 @@ #include // for qint64, qAsConst, qRound64 #include "defs.h" -#include "formspec.h" // for FormatSpecificDataList #include "grtcirc.h" // for linepart, RAD, gcdist, radtomiles #include "src/core/datetime.h" // for DateTime #include "src/core/logging.h" // for Fatal @@ -41,121 +40,113 @@ void InterpolateFilter::process() { - RouteList backuproute; + if (((opt_route != nullptr) && (route_count() == 0)) || ((opt_route == nullptr) && (track_count() == 0))) { + fatal(FatalMsg() << MYNAME ": Found no routes or tracks to operate on."); + } + + auto process_rte_lambda = [this](const route_head* rte)->void { + process_rte(const_cast(rte)); + }; if (opt_route != nullptr) { - route_swap(backuproute); + route_disp_all(process_rte_lambda, nullptr, nullptr); } else { - track_swap(backuproute); + track_disp_all(process_rte_lambda, nullptr, nullptr); } +} - if (backuproute.empty()) { - fatal(FatalMsg() << MYNAME ": Found no routes or tracks to operate on."); +void InterpolateFilter::process_rte(route_head* rte) +{ + // Steal all the wpts + WaypointList wptlist; + if (opt_route != nullptr) { + route_swap_wpts(rte, wptlist); + } else { + track_swap_wpts(rte, wptlist); } - for (const auto* rte_old : qAsConst(backuproute)) { - // FIXME: Allocating a new route_head and copying the members one at a - // time is not maintainable. When new members are added it is likely - // they will not be copied here! - // We want a deep copy of everything but with an empty WaypointList. - auto* rte_new = new route_head; - rte_new->rte_name = rte_old->rte_name; - rte_new->rte_desc = rte_old->rte_desc; - rte_new->rte_urls = rte_old->rte_urls; - rte_new->rte_num = rte_old->rte_num; - rte_new->fs = rte_old->fs.FsChainCopy(); - rte_new->line_color = rte_old->line_color; - rte_new->line_width = rte_old->line_width; - rte_new->session = rte_old->session; - if (opt_route != nullptr) { - route_add_head(rte_new); + // And add them back, with interpolated points interspersed. + double lat1 = 0; + double lon1 = 0; + double altitude1 = unknown_alt; + gpsbabel::DateTime time1; + bool first = true; + foreach (Waypoint* wpt, wptlist) { + if (first) { + first = false; } else { - track_add_head(rte_new); - } + std::optional timespan; + if (wpt->creation_time.isValid() && time1.isValid()) { + timespan = time1.msecsTo(wpt->creation_time); + } + std::optional altspan; + if (altitude1 != unknown_alt && wpt->altitude != unknown_alt) { + altspan = wpt->altitude - altitude1; + } - double lat1 = 0; - double lon1 = 0; - double altitude1 = unknown_alt; - gpsbabel::DateTime time1; - bool first = true; - for (const Waypoint* wpt : rte_old->waypoint_list) { - if (first) { - first = false; - } else { - std::optional timespan; - if (wpt->creation_time.isValid() && time1.isValid()) { - timespan = time1.msecsTo(wpt->creation_time); - } - std::optional altspan; - if (altitude1 != unknown_alt && wpt->altitude != unknown_alt) { - altspan = wpt->altitude - altitude1; + // How many points need to be inserted? + double npts = 0; + if (opt_time != nullptr) { + if (!timespan.has_value()) { + fatal(FatalMsg() << MYNAME ": points must have valid times to interpolate by time!"); } + // interpolate even if time is running backwards. + npts = std::abs(*timespan) / max_time_step; + } else if (opt_dist != nullptr) { + double distspan = radtomiles(gcdist(RAD(lat1), + RAD(lon1), + RAD(wpt->latitude), + RAD(wpt->longitude))); + npts = distspan / max_dist_step; + } + if (!std::isfinite(npts) || (npts >= INT_MAX)) { + fatal(FatalMsg() << MYNAME ": interpolation interval too small!"); + } - // How many points need to be inserted? - double npts = 0; - if (opt_time != nullptr) { - if (!timespan.has_value()) { - fatal(FatalMsg() << MYNAME ": points must have valid times to interpolate by time!"); - } - // interpolate even if time is running backwards. - npts = std::abs(*timespan) / max_time_step; - } else if (opt_dist != nullptr) { - double distspan = radtomiles(gcdist(RAD(lat1), - RAD(lon1), - RAD(wpt->latitude), - RAD(wpt->longitude))); - npts = distspan / max_dist_step; + // Insert the required points + int nmax = static_cast(ceil(npts)) - 1; // # of points to insert + for (int n = 0; n < nmax; ++n) { + double frac = static_cast(n + 1) / + static_cast(nmax + 1); + // We create the inserted point from the Waypoint at the end of the + // span. Another choice would be the Waypoint at the beginning of + // the span. We clear some fields but use a copy of the rest or the + // interpolated value. + auto* wpt_new = new Waypoint(*wpt); + wpt_new->shortname = QString(); + wpt_new->description = QString(); + if (timespan.has_value()) { + wpt_new->SetCreationTime(time1.addMSecs(qRound64(frac * *timespan))); + } else { + wpt_new->creation_time = gpsbabel::DateTime(); } - if (!std::isfinite(npts) || (npts >= INT_MAX)) { - fatal(FatalMsg() << MYNAME ": interpolation interval too small!"); + linepart(lat1, lon1, + wpt->latitude, wpt->longitude, + frac, + &wpt_new->latitude, + &wpt_new->longitude); + if (altspan.has_value()) { + wpt_new->altitude = altitude1 + (frac * *altspan); + } else { + wpt_new->altitude = unknown_alt; } - - // Insert the required points - int nmax = static_cast(ceil(npts)) - 1; // # of points to insert - for (int n = 0; n < nmax; ++n) { - double frac = static_cast(n + 1) / - static_cast(nmax + 1); - // We create the inserted point from the Waypoint at the end of the - // span. Another choice would be the Waypoint at the beginning of - // the span. We clear some fields but use a copy of the rest or the - // interpolated value. - auto* wpt_new = new Waypoint(*wpt); - wpt_new->shortname = QString(); - wpt_new->description = QString(); - if (timespan.has_value()) { - wpt_new->SetCreationTime(time1.addMSecs(qRound64(frac * *timespan))); - } else { - wpt_new->creation_time = gpsbabel::DateTime(); - } - linepart(lat1, lon1, - wpt->latitude, wpt->longitude, - frac, - &wpt_new->latitude, - &wpt_new->longitude); - if (altspan.has_value()) { - wpt_new->altitude = altitude1 + (frac * *altspan); - } else { - wpt_new->altitude = unknown_alt; - } - if (opt_route != nullptr) { - route_add_wpt(rte_new, wpt_new); - } else { - track_add_wpt(rte_new, wpt_new); - } + if (opt_route != nullptr) { + route_add_wpt(rte, wpt_new); + } else { + track_add_wpt(rte, wpt_new); } } - if (opt_route != nullptr) { - route_add_wpt(rte_new, new Waypoint(*wpt)); - } else { - track_add_wpt(rte_new, new Waypoint(*wpt)); - } - - lat1 = wpt->latitude; - lon1 = wpt->longitude; - altitude1 = wpt->altitude; - time1 = wpt->creation_time.toUTC(); // use utc to avoid tz conversions. } + if (opt_route != nullptr) { + route_add_wpt(rte, wpt); + } else { + track_add_wpt(rte, wpt); + } + + lat1 = wpt->latitude; + lon1 = wpt->longitude; + altitude1 = wpt->altitude; + time1 = wpt->creation_time.toUTC(); // use utc to avoid tz conversions. } - backuproute.flush(); } void InterpolateFilter::init() diff --git a/interpolate.h b/interpolate.h index 5df73a8ea..9f09ac5ee 100644 --- a/interpolate.h +++ b/interpolate.h @@ -1,7 +1,7 @@ /* Interpolate filter - Copyright (C) 2002 Robert Lipe, robertlipe+source@gpsbabel.org + Copyright (C) 2002,2023 Robert Lipe, robertlipe+source@gpsbabel.org This program is free software; you can redistribute it and/or modify it under the terms of the GNU General Public License as published by @@ -41,6 +41,12 @@ public: void process() override; private: + /* Member Functions */ + + void process_rte(route_head* rte); + + /* Data Members */ + char* opt_time{nullptr}; double max_time_step{0}; char* opt_dist{nullptr}; diff --git a/resample.cc b/resample.cc index 251576b69..a7eb97313 100644 --- a/resample.cc +++ b/resample.cc @@ -1,7 +1,7 @@ /* Track resampling filter - Copyright (C) 2021 Robert Lipe, robertlipe+source@gpsbabel.org + Copyright (C) 2021,2023 Robert Lipe, robertlipe+source@gpsbabel.org This program is free software; you can redistribute it and/or modify it under the terms of the GNU General Public License as published by @@ -23,6 +23,7 @@ #include // for round #include // for optional +#include // for tuple, tuple_element<>::type #include // for QDebug #include // for QList<>::const_iterator @@ -31,7 +32,6 @@ #include // for qDebug, qAsConst, qint64 #include "defs.h" // for Waypoint, route_head, fatal, WaypointList, track_add_wpt, track_disp_all, RouteList, track_add_head, track_del_wpt, track_swap, UrlList, gb_color, global_options, global_opts -#include "formspec.h" // for FormatSpecificDataList #include "src/core/datetime.h" // for DateTime #include "src/core/logging.h" // for FatalMsg #include "src/core/nvector.h" // for NVector @@ -126,85 +126,80 @@ void ResampleFilter::average_waypoint(Waypoint* wpt, bool zero_stuffed) counter = (counter + 1) % average_count; } -void ResampleFilter::process() +void ResampleFilter::interpolate_rte(route_head* rte) { - if (interpolateopt) { - RouteList backuptrack; - track_swap(backuptrack); - - if (backuptrack.empty()) { - fatal(FatalMsg() << MYNAME ": Found no tracks to operate on."); - } + // Steal all the wpts + WaypointList wptlist; + track_swap_wpts(rte, wptlist); + + // And add them back, with zero stuffed points interspersed. + bool first = true; + const Waypoint* prevwpt; + foreach (Waypoint* wpt, wptlist) { + if (first) { + first = false; + } else { + std::optional timespan; + if (prevwpt->creation_time.isValid() && wpt->creation_time.isValid()) { + timespan = wpt->creation_time.toMSecsSinceEpoch() - + prevwpt->creation_time.toMSecsSinceEpoch(); + } - for (const auto* rte_old : qAsConst(backuptrack)) { - // FIXME: Allocating a new route_head and copying the members one at a - // time is not maintainable. When new members are added it is likely - // they will not be copied here! - // We want a deep copy of everything but with an empty WaypointList. - auto* rte_new = new route_head; - rte_new->rte_name = rte_old->rte_name; - rte_new->rte_desc = rte_old->rte_desc; - rte_new->rte_urls = rte_old->rte_urls; - rte_new->rte_num = rte_old->rte_num; - rte_new->fs = rte_old->fs.FsChainCopy(); - rte_new->line_color = rte_old->line_color; - rte_new->line_width = rte_old->line_width; - rte_new->session = rte_old->session; - track_add_head(rte_new); - - bool first = true; - const Waypoint* prevwpt; - for (const auto* wpt : rte_old->waypoint_list) { - if (first) { - first = false; + // Insert the required points + for (int n = 0; n < interpolate_count - 1; ++n) { + double frac = static_cast(n + 1) / + static_cast(interpolate_count); + // We create the inserted point from the Waypoint at the + // beginning of the span. We clear some fields but use a + // copy of the rest or the interpolated value. + auto* wpt_new = new Waypoint(*prevwpt); + wpt_new->wpt_flags.new_trkseg = 0; + wpt_new->shortname = QString(); + wpt_new->description = QString(); + if (timespan.has_value()) { + wpt_new->SetCreationTime(0, prevwpt->creation_time.toMSecsSinceEpoch() + + round(frac * *timespan)); } else { - std::optional timespan; - if (prevwpt->creation_time.isValid() && wpt->creation_time.isValid()) { - timespan = wpt->creation_time.toMSecsSinceEpoch() - - prevwpt->creation_time.toMSecsSinceEpoch(); - } - - { - auto* newwpt = new Waypoint(*const_cast(prevwpt)); - newwpt->extra_data = nullptr; - track_add_wpt(rte_new, newwpt); - } - // Insert the required points - for (int n = 0; n < interpolate_count - 1; ++n) { - double frac = static_cast(n + 1) / - static_cast(interpolate_count); - // We create the inserted point from the Waypoint at the - // beginning of the span. We clear some fields but use a - // copy of the rest or the interpolated value. - auto* wpt_new = new Waypoint(*prevwpt); - wpt_new->wpt_flags.new_trkseg = 0; - wpt_new->shortname = QString(); - wpt_new->description = QString(); - if (timespan.has_value()) { - wpt_new->SetCreationTime(0, prevwpt->creation_time.toMSecsSinceEpoch() + - round(frac * *timespan)); - } else { - wpt_new->creation_time = gpsbabel::DateTime(); - } - // zero stuff - wpt_new->latitude = 0.0; - wpt_new->longitude = 0.0; - wpt_new->altitude = 0.0; - wpt_new->extra_data = &wpt_zero_stuffed; - track_add_wpt(rte_new, wpt_new); - } - - if (wpt == rte_old->waypoint_list.back()) { - auto* newwpt = new Waypoint(*const_cast(wpt)); - newwpt->extra_data = nullptr; - track_add_wpt(rte_new, newwpt); - } + wpt_new->creation_time = gpsbabel::DateTime(); } - - prevwpt = wpt; + // zero stuff + wpt_new->latitude = 0.0; + wpt_new->longitude = 0.0; + wpt_new->altitude = 0.0; + wpt_new->extra_data = &wpt_zero_stuffed; + track_add_wpt(rte, wpt_new); } } - backuptrack.flush(); + wpt->extra_data = nullptr; + track_add_wpt(rte, wpt); + + prevwpt = wpt; + } +} + +void ResampleFilter::decimate_rte(const route_head* rte) +{ + int index = 0; + foreach (Waypoint* wpt, rte->waypoint_list) { + if (index % decimate_count != 0) { + wpt->wpt_flags.marked_for_deletion = 1; + } + ++index; + } + track_del_marked_wpts(const_cast(rte)); +} + +void ResampleFilter::process() +{ + if (interpolateopt) { + if (track_count() == 0) { + fatal(FatalMsg() << MYNAME ": Found no tracks to operate on."); + } + + auto interpolate_rte_lambda = [this](const route_head* rte)->void { + interpolate_rte(const_cast(rte)); + }; + track_disp_all(interpolate_rte_lambda, nullptr, nullptr); } if (averageopt) { @@ -232,50 +227,14 @@ void ResampleFilter::process() } if (decimateopt) { - // This is ~20x faster than deleting the points in the existing route one at a time. - RouteList backuptrack; - track_swap(backuptrack); - - if (backuptrack.empty()) { + if (track_count() == 0) { fatal(FatalMsg() << MYNAME ": Found no tracks to operate on."); } - for (const auto* rte_old : qAsConst(backuptrack)) { - // FIXME: Allocating a new route_head and copying the members one at a - // time is not maintainable. When new members are added it is likely - // they will not be copied here! - // We want a deep copy of everything but with an empty WaypointList. - auto* rte_new = new route_head; - rte_new->rte_name = rte_old->rte_name; - rte_new->rte_desc = rte_old->rte_desc; - rte_new->rte_urls = rte_old->rte_urls; - rte_new->rte_num = rte_old->rte_num; - rte_new->fs = rte_old->fs.FsChainCopy(); - rte_new->line_color = rte_old->line_color; - rte_new->line_width = rte_old->line_width; - rte_new->session = rte_old->session; - track_add_head(rte_new); - - bool newseg = false; - int index = 0; - for (const auto* wpt : rte_old->waypoint_list) { - if (index % decimate_count == 0) { - auto* newwpt = new Waypoint(*const_cast(wpt)); - if (newseg) { - newwpt->wpt_flags.new_trkseg = 1; - } - track_add_wpt(rte_new, newwpt); - newseg = false; - } else { - // carry any new track segment marker forward. - if (wpt->wpt_flags.new_trkseg) { - newseg = true; - } - } - ++index; - } - } - backuptrack.flush(); + auto decimate_rte_lambda = [this](const route_head* rte)->void { + decimate_rte(rte); + }; + track_disp_all(decimate_rte_lambda, nullptr, nullptr); } } diff --git a/resample.h b/resample.h index 51cb0b832..9545f187d 100644 --- a/resample.h +++ b/resample.h @@ -1,7 +1,7 @@ /* Track resampling filter - Copyright (C) 2021 Robert Lipe, robertlipe+source@gpsbabel.org + Copyright (C) 2021,2023 Robert Lipe, robertlipe+source@gpsbabel.org This program is free software; you can redistribute it and/or modify it under the terms of the GNU General Public License as published by @@ -24,6 +24,7 @@ #include // for tuple +#include // for QString #include // for QVector #include "defs.h" // for arglist_t, ARGTYPE_INT, Waypoint, route_head @@ -46,7 +47,13 @@ public: private: + /* Member Functions */ + void average_waypoint(Waypoint* wpt, bool zero_stuffed); + void interpolate_rte(route_head* rte); + void decimate_rte(const route_head* rte); + + /* Data Members */ QVector> history; gpsbabel::NVector accumulated_position; -- 2.30.2